This repository was archived by the owner on Dec 10, 2024. It is now read-only.
Add support for hidden field to project variables#2065
Merged
RicePatrick merged 3 commits intoxanzy:mainfrom Dec 9, 2024
Merged
Add support for hidden field to project variables#2065RicePatrick merged 3 commits intoxanzy:mainfrom
RicePatrick merged 3 commits intoxanzy:mainfrom
Conversation
timofurrer
previously requested changes
Nov 21, 2024
Contributor
timofurrer
left a comment
There was a problem hiding this comment.
@yogeshlonkar thanks for the contribution 🤝 I've left some clarification questions on the correctness of the field / field naming. Back to you 🏓
RicePatrick
suggested changes
Dec 4, 2024
Contributor
RicePatrick
left a comment
There was a problem hiding this comment.
I went ahead and tested this against a locally running GitLab with the 2 suggested changes, and the tests pass. If you'd please make those changes, I'm good to merge.
Contributor
|
Here is the test I used to test the above 2 suggestions: func TestProjectVariablesService_MaskAndHidden_RealGitLab(t *testing.T) {
client, err := NewClient("<token>",
WithBaseURL("http://localhost:8085"),
)
if err != nil {
t.Fatalf("Failed to create client: %v", err)
}
// create a project
project, _, err := client.Projects.CreateProject(&CreateProjectOptions{
Name: Ptr(fmt.Sprintf("test-%d", time.Now().UnixMicro())),
})
if err != nil {
t.Fatalf("Failed to create project: %v", err)
}
projectVar, _, err := client.ProjectVariables.CreateVariable(project.ID, &CreateProjectVariableOptions{
Key: Ptr("test"),
Value: Ptr("testtesttest"), // must have a min of 8 characters
MaskedAndHidden: Ptr(true),
})
if err != nil {
t.Fatalf("Failed to create project variable: %v", err)
}
assert.True(t, projectVar.Hidden)
} |
GitLab has inconsistent naming of the filed for hidden variables. It seems for keeping UI consistant, GitLab chose to introduce different naming for the same field. This could have been easy handled in UI by setting two variables on API request instead of introducing combined field for UI. I have added 2 test cases for create and update project API as per the gitlab.com API that I manually tested and added similar unit tests.
Contributor
Author
|
@RicePatrick @timofurrer can you please review PR again |
Contributor
|
Updated code looks good; merging so it's available for the migration tomorrow :) |
RicePatrick
approved these changes
Dec 9, 2024
Changes made as part of further commits
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitLab has inconsistent naming of the filed for hidden variables.
It seems for keeping UI consistant, GitLab chose to introduce different
naming for the same field. This could have been easy handled in UI by
setting two variables on API request instead of introducing combined
field for UI.
I have added 2 test cases for create and update project API as per the
gitlab.com API that I manually tested and added similar unit tests.